Skip to content

Conversation

@tadhgboyle
Copy link
Member

@tadhgboyle tadhgboyle commented May 24, 2025

image image

@partydragen
Copy link
Member

Amazing work!

Add a option to set automaticly reschedule backup task daily
Maybe even option to delete all backups after X latest backups

Display warning on update page to backup site first? :D

@tadhgboyle
Copy link
Member Author

Amazing work!

Add a option to set automaticly reschedule backup task daily Maybe even option to delete all backups after X latest backups

Display warning on update page to backup site first? :D

Added those settings @partydragen!
Will add a link to create a backup on the update page too, thanks for the suggestions!

@tadhgboyle tadhgboyle requested a review from partydragen May 25, 2025 19:08
@tadhgboyle tadhgboyle requested a review from Copilot May 25, 2025 19:22
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds a full backup feature including UI integration, scheduled/manual backups, and retention settings.

  • Integrates backup prompts in the update panel and maintenance dashboard.
  • Introduces a dedicated backups page for creation, download, and configuration.
  • Implements the Backup task, adjusts permissions in migrations/initialiser, and adds supporting utilities and language entries.

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.

Show a summary per file
File Description
modules/Core/pages/panel/update.php Display backup recommendation in update panel
modules/Core/pages/panel/debugging_and_maintenance.php Add backups link to maintenance panel
modules/Core/pages/panel/backups.php New backup management UI and actions
modules/Core/module.php Register backups page and permission
modules/Core/language/en_UK.json Add translation strings for backup feature
modules/Core/classes/Tasks/Backup.php Backup creation, scheduling, and cleanup logic
core/includes/updates/230.php Migration to grant backup permission to Admin group
core/classes/Database/DatabaseInitialiser.php Initialize backup permission for default Admin group
core/classes/Core/Util.php Add human-readable byte formatting
composer.json Add mysqldump PHP library dependency

@Derkades
Copy link
Member

We must be very careful to deny access to /backups in .htaccess and the nginx configuration file. Maybe Nameless can try making a request to /backups/somefile, and refuse to make a backup if it succeeds?

@tadhgboyle
Copy link
Member Author

We must be very careful to deny access to /backups in .htaccess and the nginx configuration file. Maybe Nameless can try making a request to /backups/somefile, and refuse to make a backup if it succeeds?

I changed this so that it stores backups in /cache/backups, I think this is much safer since we already guard the /cache folder from being publicly accessible - what do you think?
FWIW, in the near future I'd love to rename this to /storage, and have things like /storage/cache, /storage/backups, /storage/logs, etc

@tadhgboyle tadhgboyle requested a review from Derkades June 6, 2025 06:15
Copy link
Member

@partydragen partydragen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should prop have sam review this too, Dont want any security leaks of this,

Also are we sure backup folder is properly protected that people cant guess the link to download the zip directly

Like i see it uses time hard to guess exact time but if its displayed in staffcp then any staff might find out about the direct download link

if (!isset($permissions['admincp.core.backups'])) {
$permissions['admincp.core.backups'] = 1;
DB::getInstance()->update('groups', $admin_group->id, [
'permissions' => json_encode($permissions),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of this can still be removed, We have not updated admin group perms for ages as it already had Adminstrator perm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants